Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

build: add Dockerfile for controller, build with bottlerocket-sdk #85

Merged
merged 2 commits into from
Aug 24, 2021
Merged

build: add Dockerfile for controller, build with bottlerocket-sdk #85

merged 2 commits into from
Aug 24, 2021
+235 −198

Conversation

etungsten
Copy link
Contributor

@etungsten etungsten commented Aug 19, 2021 •
edited
Loading

Note: Depends on #84

Issue number:
Closes #11

Description of changes:

commit cab144a3efc5675b75ea33c482a9f6798ff857fa
Author: Erikson Tung <[email protected]>
Date:   Mon Aug 23 15:22:47 2021 -0700

    dependency: use default kube-rs features, disable rustls


Author: Erikson Tung <[email protected]>
Date:   Thu Aug 19 13:54:29 2021 -0700

    build: add Dockerfile for controller, build with bottlerocket-sdk
    
    Build the binaries with the bottlerocket-sdk.
    
    Dockerfile: install openssl with musl for controller, test-agent

Testing done:
Able to build the example-test-agent and the controller and run the binaries with their respective container images.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@etungsten etungsten changed the title Build with sdk build: add Dockerfile for controller, build with bottlerocket-sdk Aug 19, 2021
@webern webern added the in-review A PR is open that will close this issue label Aug 19, 2021
@webern webern added this to the sprint-number-1 milestone Aug 19, 2021
webern
webern approved these changes Aug 19, 2021
View reviewed changes
Copy link
Contributor

@webern webern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might needs something like this to make TLS implementations (whether OpenSSL or Rustls) happy https://github.com/bottlerocket-os/bottlerocket-ecs-updater/blob/develop/Dockerfile#L34..L35

Obviously merge #84 first, but this looks fine. Thanks!

controller/Dockerfile Outdated Show resolved Hide resolved
@etungsten
Copy link
Contributor Author

Push above adds the CA certificates store to the container images.

@jpculp
Copy link
Member

jpculp commented Aug 23, 2021

This seems fine to me after #84 is merged.

@jpculp jpculp self-requested a review August 23, 2021 15:51
@etungsten
Copy link
Contributor Author

Push above rebases onto develop.

@etungsten etungsten marked this pull request as ready for review August 23, 2021 17:42
arnaldo2792
arnaldo2792 reviewed Aug 23, 2021
View reviewed changes
controller/Dockerfile Outdated Show resolved Hide resolved
test-agent/examples/example_test_agent/Dockerfile Outdated Show resolved Hide resolved
@etungsten
Copy link
Contributor Author

Push above changes a few things:

Also removed redundant WORKDIR as pointed out by @arnaldo2792

webern
webern reviewed Aug 23, 2021
View reviewed changes
Copy link
Contributor

@webern webern left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should build an image tagged e.g. bottlerocket-sdk-with-openssl and then use it in both FROM lines. This deduplicates the code. I don't think we need to push the tagged image anywhere, but the makefile can ensure that other docker builds depend on it so we know it will be built and available locally.

@webern
Copy link
Contributor

webern commented Aug 23, 2021

depend on it so we know it will be built and available locally.

We might also see if Ben thinks the use case seems likely enough to warrant including musl openssl-devl in the SDK itself, which would be ideal.

webern
webern reviewed Aug 24, 2021
View reviewed changes
Makefile Outdated Show resolved Hide resolved
@webern webern self-requested a review August 24, 2021 18:13
@etungsten
Copy link
Contributor Author

Push above separates out the openssl musl build into a separate Dockerfile that creates an augmented bottlerocket-sdk with openssl built with the musl toolchain that we use for building the other container images.

@etungsten
Copy link
Contributor Author

Push above changes some Makefile target names as recommended by @webern

webern
webern reviewed Aug 24, 2021
View reviewed changes
Makefile Show resolved Hide resolved
webern
webern reviewed Aug 24, 2021
View reviewed changes
Makefile Outdated Show resolved Hide resolved
@etungsten
Copy link
Contributor Author

etungsten commented Aug 24, 2021 •
edited
Loading

Push above relocates the OPENSSL_* env vars to the sdk-openssl image. Also some minor changes to names.
Push below changes test-sys to testsys and removes --network=host for docker builds.

Images still build fine.

webern
webern approved these changes Aug 24, 2021
View reviewed changes
Copy link
Contributor

@webern webern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍰

arnaldo2792
arnaldo2792 approved these changes Aug 24, 2021
View reviewed changes
Copy link
Contributor

@arnaldo2792 arnaldo2792 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits, but approved 👍

Dockerfile.sdk_openssl Outdated Show resolved Hide resolved
Dockerfile.sdk_openssl Outdated Show resolved Hide resolved
Dockerfile.sdk_openssl
Comment on lines +26 to +29
ENV PKG_CONFIG_ALLOW_CROSS=1
ENV OPENSSL_STATIC=true
ENV OPENSSL_DIR=/musl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Shouldn't ENVs be closer to ARGs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's absolutely necessary since it's not used for building openssl. These are environment variables we should set after we're able to successfully build openssl with musl. It's intended for building the openssl-sys crate later when we build the controller image. I'll add a comment here to indicate what these environment variables are for.

@etungsten
build: add Dockerfile for controller, build with bottlerocket-sdk
834f37d 
Build the binaries with the bottlerocket-sdk.

Dockerfile: install openssl with musl for controller, test-agent
@etungsten
Copy link
Contributor Author

etungsten commented Aug 24, 2021 •
edited
Loading

Push above and below addresses @arnaldo2792 's comments.

Tested the builds and they still work.

arnaldo2792
arnaldo2792 approved these changes Aug 24, 2021
View reviewed changes
Copy link
Contributor

@arnaldo2792 arnaldo2792 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

@etungsten etungsten merged commit a84e9b8 into develop Aug 24, 2021
@etungsten etungsten deleted the build-with-sdk branch August 24, 2021 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arnaldo2792 arnaldo2792 arnaldo2792 approved these changes

@webern webern webern approved these changes

@jpculp jpculp Awaiting requested review from jpculp

Assignees
No one assigned
Labels
in-review A PR is open that will close this issue
Projects
None yet
Milestone
sprint-01
Development

Successfully merging this pull request may close these issues.

build: create controller image
4 participants
@etungsten @jpculp @webern @arnaldo2792